Skip to content

Redirect tl_expected to system libexpected-dev when available#12

Open
otamachan wants to merge 9 commits intoPickNikRobotics:mainfrom
otamachan:redirect-tl-expected-to-system-header
Open

Redirect tl_expected to system libexpected-dev when available#12
otamachan wants to merge 9 commits intoPickNikRobotics:mainfrom
otamachan:redirect-tl-expected-to-system-header

Conversation

@otamachan
Copy link

This PR updates tl_expected/expected.hpp to redirect to the system tl/expected.hpp when available

  • When libexpected-dev is installed (detected via __has_include), tl_expected/expected.hpp redirects to the system tl/expected.hpp with a deprecation warning
  • When libexpected-dev is not available (e.g., Windows), the vendored implementation is used as before
  • Users who need the vendored version can opt in with -DUSE_VENDORED_TL_EXPECTED=ON
  • Added libexpected-dev as build_export_depend to ensure rosdep installs the system package

Related to

Signed-off-by: Tamaki Nishino <otamachan@gmail.com>
@christophfroehlich christophfroehlich linked an issue Feb 28, 2026 that may be closed by this pull request
@christophfroehlich
Copy link
Collaborator

christophfroehlich commented Feb 28, 2026

At first place I was wondering why I couldn't see your deprecation warning building RSL against your branch: It picked the tl_expected header from rcpputils. I did not know before that this was packaged there. Should we maybe simply remove it here and let ROS PMC handle this in rcpputils?

Changing the includes from
#include <tl_expected/expected.hpp>
to
#include "tl_expected/expected.hpp"
used that of my local workspace and had the desired deprecation warning.

@otamachan
Copy link
Author

Thanks for the insight.
Looking into this, it seems rcpputils/tl_expected/expected.hpp was added to internalize the dependency for point_cloud_transport as a core package (ros2/ros2#1516, https://github.com/ros-perception/point_cloud_transport/pull/48/changes). It's not referenced from any of rcpputils' own public headers — it's only used via #include <rcpputils/tl_expected/expected.hpp> in point_cloud_transport's public headers. So the number of external packages depending on it directly should be small, though including point_cloud_transport headers would still trigger the same header guard conflict.

On the other hand, cpp_polyfills' tl_expected has direct downstream dependents, so it can't simply be removed.

Regarding the deprecation warning not appearing — I'll investigate later.

@christophfroehlich

This comment was marked as outdated.

@christophfroehlich
Copy link
Collaborator

Sorry, I tried now to just include rcpputils, and it did not work without adapting the includes to <rcpputils/..> as you pointed out: PickNikRobotics/RSL#157

otamachan and others added 3 commits March 1, 2026 20:44
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Signed-off-by: Tamaki Nishino <otamachan@gmail.com>
Copy link
Collaborator

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we remove USE_VENDORED_TL_EXPECTED and just delete the code?
At least this was my proposal in PickNikRobotics/generate_parameter_library#311 (comment) (open for discussion).

Apart from that, the deprecation warning and redirection works as expected. Tested with PickNikRobotics/generate_parameter_library#322 and joint_trajectory_controller

@otamachan
Copy link
Author

The USE_VENDORED_TL_EXPECTED option was originally added for users who depend on a newer version of tl_expected (PickNikRobotics/generate_parameter_library#311 (comment)). However, since tl_expected is no longer receiving new releases and it should be backward compatible, I think it's safe to remove this option. I'll update the PR accordingly.

@otamachan otamachan force-pushed the redirect-tl-expected-to-system-header branch from f2bb765 to 9d6b028 Compare March 12, 2026 06:05
Copy link
Collaborator

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Collaborator

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No wait, shouldn't the deprecation warning also be given if
__has_include(<tl/expected.hpp>)
is false? On the long run we want to remove this package at all, right?

@otamachan otamachan force-pushed the redirect-tl-expected-to-system-header branch from 9d6b028 to 953183f Compare March 12, 2026 08:27
@otamachan
Copy link
Author

Sorry, the deprecation warning logic was reversed in the previous push. Fixed now.

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redirect to system tl-expected package?

2 participants